Allow limiting New-PI credit to partner institutions #96
Conversation
QuanMPhm
force-pushed
the
94/limit_credits
branch
from
0d07205
to
ea5911e
Compare
process_report/institute_list.yaml
Outdated
| - display_name: Boston University | ||
| domains: | ||
| - bu.edu | ||
| - robbaron |
There was a problem hiding this comment.
Since we've fully transitioned to using email addresses rather than username (and also have a aliasin system in place), I feel it's safe to remove everything that is an username from this file.
QuanMPhm
force-pushed
the
94/limit_credits
branch
from
ea5911e
to
ce71522
Compare
| "mghpcc_partnership_start_date", None | ||
| ): | ||
| if ( | ||
| util.get_month_diff(self.invoice_month, partnership_start_date[:-3]) |
There was a problem hiding this comment.
Usually I'd add a comment when you need to access some very specific indexes of a string.
In this case, however, I think we should convert it to a datetime object and then get the month and year.
>>> "{:%Y-%m}".format(datetime.strptime("2024-09-01", "%Y-%m-%d"))
'2024-09'
This would also work in the case where someone writes the date as "2024-9-1" in institute_list.yaml because nothing is enforcing that a zero is added.
>>> "{:%Y-%m}".format(datetime.strptime("2024-9-1", "%Y-%m-%d"))
'2024-09'
as opposed to
>>> "2024-9-1"[:-3]
'2024-'
745ecff
to
0cf4f8f
Compare
As prerequisite to adding metadata about our list of institutions, such as whether MGHPCC has a partnership with them, and to make the file more human-friendly, `institute.json` has been converted to a YAML and formatted as a list. Each element of the YAML list must be a dict with 2 attributes: - `display_name`: The name of the institute, as will appear on invoices - `domains`: A list containing domains for that institute Additional metadata can be freely added to each institute dict as our billing needs change. There is also some small cleanup. Namely, some functions in `process_report.py` that have been moved to `util.py` were not removed during refactoring.
QuanMPhm
force-pushed
the
94/limit_credits
branch
from
0cf4f8f
to
3121ac7
Compare
There was a problem hiding this comment.
One comment regarding the format of the partnership start date, otherwise looks good.
Also please note that since no institutions in this list have a partnership start date, in the current cycle no one would receive a credit unless we introduce those dates in a subsequent PR.
| "mghpcc_partnership_start_date" | ||
| ): | ||
| partnership_year_month = "{:%Y-%m}".format( | ||
| datetime.strptime(partnership_start_date, "%Y-%m-%d") |
There was a problem hiding this comment.
I understand you're following my suggestion of the partnership start date being YYYY-MM-DD, but considering that we're working with YYYY-MM in every other aspect of the billing pipeline (including nerc-rates) I think it's reasonable to just use YYYY-MM here too. So no need to format, just pass the value from the yaml to get_month_diff
There was a problem hiding this comment.
@knikolla I think that was because of my concerns described in this comment: #96 (comment)
There was a problem hiding this comment.
but yeah, if it's just year and month then it could probably be passed directly.
There was a problem hiding this comment.
The date format of the partnership start date will now be YYYY-MM. I've updated the PR message accordingly.
`process_report.py` will now use `nerc_rates` to determine if the New-PI credit is limited to institutions that are MGHPCC partners for the given invoice month. The script determines whether an institution's MGHPCC partnership is active by first seeing if the `mghpcc_partnership_start_date` field is set in `institute_list.yaml`, then checks if the start date is within or before the invoice month. Also removed usernames from `institute_list.yaml`, as they are no longer nessecary given the alias validation step.
QuanMPhm
force-pushed
the
94/limit_credits
branch
from
3121ac7
to
696407c
Compare
There was a problem hiding this comment.
looks fine to me.
There is also some small cleanup. Namely, some functions in
process_report.py
that have been moved toutil.pywere not removed during refactoring.
What was the small cleanup? whatever was moved from process_report.py to util.py in this PR seemed to be relevant?
|
@naved001 During the first refactor PR (#54) when the Functionally, this didn't change anything. Stylistically, it wasn't a big deal until this PR, since more references to these functions were added. While writing this PR, I just decided the two functions should be placed in |
In the future I'd recommend separate commits for tangentially related cleanups. |
Closes #94,
process_report.pywill now usenerc_ratesto determine if the New-PI credit is limited to institutions that are MGHPCC partners for the given invoice month.The script determines whether an institution's MGHPCC partnership is active by first seeing if the
mghpcc_partnership_start_datefield is set ininstitute_list.yaml, then checks if the start date is within or before the invoice month.@joachimweyl To mark an institution as an MGHPCC partner, you will need to set the field
mghpcc_partnership_start_dateininstitute_list.yamlto the start date of the partnership in YYYY-MM format.I.e for Boston University, their current entry looks like this:
To mark them as a partner, change their entry like so: